Process Fucntion refactor to Enforce return_jacp_stacked=True#5006
Process Fucntion refactor to Enforce return_jacp_stacked=True#5006RohitP2005 wants to merge 1 commit intopybamm-team:mainfrom
Conversation
|
@martinjrobins Thanks! |
martinjrobins
left a comment
martinjrobins
left a comment
There was a problem hiding this comment.
Hi @RohitP2005, I think there is much more to do on this (than just removing the arg. Have you run the tests locally?
| else: | ||
| jac = None | ||
| jac_action = None | ||
| jac = func.get_jacobian() if use_jacobian else None |
There was a problem hiding this comment.
I think the single if statement is less complex then multiple, plus you are removing the report, so I'd disagree with this change
src/pybamm/solvers/base_solver.py
Outdated
| else: | ||
| jacp = None | ||
|
|
||
| if use_jacobian: |
| f"Calculating sensitivities for {name} with respect " | ||
| f"to parameters {model.calculate_sensitivities} using " | ||
| "CasADi" | ||
| report(f"Calculating sensitivities for {name} using CasADi") |
There was a problem hiding this comment.
its useful to have the printout of model.calculate_sensitivities here, so keep this
src/pybamm/solvers/base_solver.py
Outdated
| ) | ||
| # Compute derivate wrt p-stacked (can be passed to solver to | ||
| # compute sensitivities online) | ||
| if return_jacp_stacked: |
There was a problem hiding this comment.
removing this if statement is only a small part of this change, you will also need to change the behaviour of the solvers that depend on this functionality (I believe, but I'll run the tests so we can check)
Yes , I ran the tests locally and there are errors that needs to addressed. |
2 participants
Description
Refactored the process function in
base_solver.pyto simplify its logic and improve maintainability.Fixes #4970
Type of change
Refactor (non-breaking change that improves code structure) to the process function in the base solver to enforce
return_jacp_stacked=True.Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests